[Mono.Android] Enumify API-37.0, API-37.0 is "Stable"#11151
[Mono.Android] Enumify API-37.0, API-37.0 is "Stable"#11151jonathanpeppers merged 5 commits intodotnet:mainfrom
Conversation
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 4 issues in the API 37 enumification map.csv entries:
- ❌ Wrong enum type:
AlcoholConsumptionRecord.RECORD_TEMPORAL_TYPE_*constants are mapped toAlcoholConsumptionBeverageTypeinstead of a separate temporal-type enum (map.csv:3391-3393) - ❌ Typo:
KillExcessivCpuUsage→KillExcessiveCpuUsage(missing 'e') — this becomes a public API name (map.csv:9754) ⚠️ Namespace casing:Android.Content.Pm.Webappshould beAndroid.Content.PM.Webappto match existingandroid.content.pmconvention (map.csv:2208-2223)⚠️ Redundant prefixes: Several new member names include the enum type name as a prefix, inconsistent with existing members in the same enum types (6 instances acrossNetCapability,DocumentContractFlags,DocumentRootFlags,ProfilingTriggerType)
👍 The Documentation path updates (xamarin-android → dotnet/android) and metadata ns-replace additions look correct. The LIGHT_CAPABILITY_* flags fix and CarrierConfigManager ? → I decisions are good cleanup.
Review generated by android-reviewer from review guidelines.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #11151
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #11151
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Android PR Reviewer for issue #11151 · ● 26.6M
Context: http://github.com/jpobst/BindingStudio Context: dotnet#10005 Use jpobst/BindingStudio to begin enumifying API-36.1. Note: current dotnet/java-interop emits an "extra" `,` on `map.csv` output, which would make for a "noisy" diff (every line changed!). The diff size is reduced by removing trailing commas: sed 's/,$//' < src/Mono.Android/new-map.csv > src/Mono.Android/map.csv This keeps the diff to a reasonable size. Sanity check: during enumification, this should have no matches: git grep '\.[A-Z][a-z]\.' src/Mono.Android/map.csv Two-letter namespace parts should be all upper-case. TODO (lol): update `map.csv` to current BindingStudio output after this is merged. WIP. Current state: finished **Creating enums**. Need to perform: * **Extract methods that possibly need enums** * **Mapping methods** Commit [Extract constants from API][0] output. [0]: https://github.com/dotnet/android/blob/e67a3fd36ca5e24b0007a170ea77037e4437f9d3/Documentation/workflow/HowToAddNewApiLevel.md#extract-constants-from-api
1bd3059 to
1f394ea
Compare
Rename `Permissionui` to `PermissionUI`. Also work on a more "future proof" `Color` mapping implementation.
1f394ea to
373b46a
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 2 bugs and 1 suggestion across 2 files:
- ❌ Bugs & correctness:
@tinttypo in XPath — should be@name(src/Mono.Android/meta(redacted)meta(redacted) This silently breaks "ends with Tint" matching for two of the three Color/Tint rule blocks. - ❌ Bugs & correctness:
getMaxChannelWidthmapped to wrong enum typePreambleinstead ofWifiChannelWidth(src/Mono.Android/methodmap.csv:4656). - 💡 Code organization: Exact duplicate entries in
methodmap.csvforforStopwatch,forTimer,AlcoholConsumptionRecord$Builder.ctor,SymptomRecord$Builder.ctor.
👍 The pattern-based Color/Tint approach for API-37+ is a smart way to reduce manual enumification work going forward. The documentation updates with the namespace casing check (git grep) and BindingStudio workflow improvements are helpful. CI is still in progress (7/35 checks passed, 0 failures so far).
Review generated by android-reviewer from review guidelines.
Generated by Android PR Reviewer for issue #11151 · ● 4.1M
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
Pull request overview
Updates Mono.Android’s enumification inputs and generator metadata to incorporate Android API 37.0 (now treated as stable) and to continue the BindingStudio-driven enumification workflow.
Changes:
- Added API-37 enum constant mappings to
map.csvand expanded method enum mappings inmethodmap.csv. - Updated
metadatato normalize API-37 “api-since” source-file mapping to37.0, add namespace segment replacements, and add API-37+ Color/Tint pattern rules. - Refreshed enumification workflow documentation (paths, checks, and method-map handling guidance).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/Mono.Android/methodmap.csv | Removes duplicates/blank lines and appends API-37 method enum mappings. |
| src/Mono.Android/metadata | Adjusts API-37 api-since mapping, adds ns-replace rules, and adds API-37+ Color/Tint pattern-based fixups. |
| src/Mono.Android/map.csv | Adds a large set of API-37 enum constant mappings and related enum updates. |
| Documentation/workflow/HowToAddNewApiLevel.md | Updates BindingStudio instructions, repo paths, and adds validation steps for enumification output. |
| 37,android/bluetooth,BondStatus,ctor,pairingVariant, | ||
| 37,android/bluetooth,BondStatus,getPairingAlgorithm,return,Android.Bluetooth.BluetoothDevicePairingAlgorithm | ||
| 37,android/bluetooth,BondStatus,getPairingVariant,return, |
There was a problem hiding this comment.
BondStatus entries for pairingVariant have an empty enum type (trailing comma). The methodmap format expects a managed enum type in the last column; leaving it blank risks parse failures or silently skipping the intended mapping. Either provide the correct enum type for pairingVariant/getPairingVariant, or remove these rows until the enum mapping is known.
| 37,android/bluetooth,BondStatus,ctor,pairingVariant, | |
| 37,android/bluetooth,BondStatus,getPairingAlgorithm,return,Android.Bluetooth.BluetoothDevicePairingAlgorithm | |
| 37,android/bluetooth,BondStatus,getPairingVariant,return, | |
| 37,android/bluetooth,BondStatus,ctor,pairingVariant,Android.Bluetooth.BluetoothDevicePairingVariant | |
| 37,android/bluetooth,BondStatus,getPairingAlgorithm,return,Android.Bluetooth.BluetoothDevicePairingAlgorithm | |
| 37,android/bluetooth,BondStatus,getPairingVariant,return,Android.Bluetooth.BluetoothDevicePairingVariant |
| 37,android/health/connect,MatchmakingRequest,writeToParcel,flags,Android.OS.ParcelableWriteFlags | ||
| 37,android/health/connect,MatchmakingResponse,writeToParcel,flags,Android.OS.ParcelableWriteFlags | ||
| 37,android/location,GnssStatus$Builder,addSatellite,constellationType,Android.Locations.GnssConstellationType | ||
| 37,android/media,AudioFormat$Builder,setChannelAcnMask,channelAcnMask, |
There was a problem hiding this comment.
AudioFormat$Builder.setChannelAcnMask has an entry with an empty managed enum type (trailing comma). If this parameter isn't meant to be enumified yet, remove the row; otherwise fill in the intended enum type so the generator can apply it deterministically.
| 37,android/media,AudioFormat$Builder,setChannelAcnMask,channelAcnMask, |
| E,37,android/health/connect/datatypes/AlcoholConsumptionRecord.ALCOHOL_CONSUMPTION_BEVERAGE_TYPE_ABSINTHE,14,Android.Health.Connect.Datatypes.AlcoholConsumptionBeverageType,AlcoholConsumptionBeverageTypeAbsinthe,remove, | ||
| E,37,android/health/connect/datatypes/AlcoholConsumptionRecord.ALCOHOL_CONSUMPTION_BEVERAGE_TYPE_BEER,1,Android.Health.Connect.Datatypes.AlcoholConsumptionBeverageType,AlcoholConsumptionBeverageTypeBeer,remove, | ||
| E,37,android/health/connect/datatypes/AlcoholConsumptionRecord.ALCOHOL_CONSUMPTION_BEVERAGE_TYPE_BRANDY,15,Android.Health.Connect.Datatypes.AlcoholConsumptionBeverageType,AlcoholConsumptionBeverageTypeBrandy,remove, | ||
| E,37,android/health/connect/datatypes/AlcoholConsumptionRecord.ALCOHOL_CONSUMPTION_BEVERAGE_TYPE_CHUHAI,17,Android.Health.Connect.Datatypes.AlcoholConsumptionBeverageType,AlcoholConsumptionBeverageTypeChuhai,remove, | ||
| E,37,android/health/connect/datatypes/AlcoholConsumptionRecord.ALCOHOL_CONSUMPTION_BEVERAGE_TYPE_CIDER,9,Android.Health.Connect.Datatypes.AlcoholConsumptionBeverageType,AlcoholConsumptionBeverageTypeCider,remove, |
There was a problem hiding this comment.
The AlcoholConsumptionBeverageType enum members are being emitted with the enum type name duplicated into each member (e.g., AlcoholConsumptionBeverageTypeAbsinthe). This is inconsistent with surrounding enums (e.g., Moderate, Vigorous) and results in very noisy/awkward public API. Consider shortening these to just the beverage name (e.g., Absinthe, Beer, Brandy, etc.).
| E,37,android/ranging/uwb/DlTdoaRangingParams.MEASUREMENT_VERSION_1,1,Android.Ranging.Uwb.MeasurementVersion,v1,remove, | ||
| E,37,android/ranging/uwb/DlTdoaRangingParams.MEASUREMENT_VERSION_2,2,Android.Ranging.Uwb.MeasurementVersion,v2,remove, | ||
| E,37,android/ranging/uwb/DlTdoaRangingParams.MEASUREMENT_VERSION_UNKNOWN,2147483647,Android.Ranging.Uwb.MeasurementVersion,Unknown,remove, |
There was a problem hiding this comment.
Enum member names v1/v2 for Android.Ranging.Uwb.MeasurementVersion start with a lowercase letter, which is inconsistent with typical enum member casing throughout map.csv (PascalCase) and will stand out in the public API. Consider renaming these to V1/V2.
| <attr api-since="36.1" path="/api//*[contains(@merge.SourceFile,'api-36.1.xml.in')]" name="api-since">36.1</attr> | ||
| <attr api-since="37" path="/api//*[contains(@merge.SourceFile,'api-CinnamonBun.xml.in')]" name="api-since">37</attr> | ||
| <attr api-since="37" path="/api//*[contains(@merge.SourceFile,'api-37.0.xml.in')]" name="api-since">37</attr> | ||
| <attr api-since="37" path="/api//*[contains(@merge.SourceFile,'api-CinnamonBun.xml.in')]" name="api-since">37.0</attr> |
There was a problem hiding this comment.
You added an api-since mapping for api-37.xml.in, but the earlier "no-alternatives" block only covers api-CinnamonBun.xml.in and api-37.0.xml.in. If the SDK is now producing api-37.xml.in, interfaces from that source file may miss the no-alternatives setting and could reintroduce interface constants/alternatives issues. Consider adding a matching no-alternatives entry for api-37.xml.in in that block (or confirm it's not needed).
| <attr api-since="37" path="/api//*[contains(@merge.SourceFile,'api-CinnamonBun.xml.in')]" name="api-since">37.0</attr> | |
| <attr api-since="37" path="/api//*[contains(@merge.SourceFile,'api-CinnamonBun.xml.in')]" name="api-since">37.0</attr> | |
| <attr path="/api//*[contains(@merge.SourceFile,'api-37.xml.in')]" name="no-alternatives">true</attr> |
| There may be redundant duplicate entries within `methodmap.csv`. Use the **uniq**(1) | ||
| Unix app to remove duplicate entries. | ||
|
|
||
| ```sh | ||
| uniq < src/Mono.Android/methodmap.csv > m |
There was a problem hiding this comment.
The docs suggest using uniq to remove duplicate entries from methodmap.csv, but uniq only removes adjacent duplicates. If duplicates aren't consecutive (likely after appending), this won't fully de-duplicate. Consider updating the instructions to use sort | uniq (or sort -u) if order isn't important, or clarify that the file must be sorted/grouped first.
| There may be redundant duplicate entries within `methodmap.csv`. Use the **uniq**(1) | |
| Unix app to remove duplicate entries. | |
| ```sh | |
| uniq < src/Mono.Android/methodmap.csv > m | |
| There may be redundant duplicate entries within `methodmap.csv`. Use the **sort**(1) | |
| Unix app with `-u` to sort the file and remove duplicate entries. | |
| ```sh | |
| sort -u src/Mono.Android/methodmap.csv > m |
| E,1,android/media/AudioManager.STREAM_ALARM,4,Android.Media.Stream,Alarm,keep, | ||
| E,37,android/media/AudioManager.STREAM_ASSISTANT,11,Android.Media.Stream,StreamAssistant,remove, | ||
| E,5,android/media/AudioManager.STREAM_DTMF,8,Android.Media.Stream,Dtmf,keep, |
There was a problem hiding this comment.
Enum member name StreamAssistant for AudioManager.STREAM_ASSISTANT doesn't match the existing Android.Media.Stream naming pattern (e.g., Alarm, Music, Notification). This will produce an inconsistent public API surface; consider renaming the enum member to Assistant (or whatever matches the established pattern) while keeping the underlying value the same.
| E,33,android/media/AudioDeviceInfo.TYPE_BLE_BROADCAST,30,Android.Media.AudioDeviceType,BleBroadcast,remove, | ||
| E,37,android/media/AudioDeviceInfo.TYPE_BLE_CENTRAL,34,Android.Media.AudioDeviceType,TypeBleCentral,remove, | ||
| E,37,android/media/AudioDeviceInfo.TYPE_BLE_CENTRAL_BROADCAST,35,Android.Media.AudioDeviceType,TypeBleCentralBroadcast,remove, | ||
| E,31,android/media/AudioDeviceInfo.TYPE_BLE_HEADSET,26,Android.Media.AudioDeviceType,BleHeadset,remove, | ||
| E,37,android/media/AudioDeviceInfo.TYPE_BLE_HEARING_AID,33,Android.Media.AudioDeviceType,TypeBleHearingAid,remove, |
There was a problem hiding this comment.
Enum member names TypeBleCentral, TypeBleCentralBroadcast, and TypeBleHearingAid include an extra Type prefix, while other Android.Media.AudioDeviceType members use concise names (e.g., BleBroadcast, BleHeadset, BuiltinMic). Rename these members to be consistent (e.g., BleCentral, BleCentralBroadcast, BleHearingAid).
| E,29,android/view/Display$HdrCapabilities.HDR_TYPE_HDR10_PLUS,4,Android.Views.HdrType,Hdr10Plus,remove, | ||
| E,24,android/view/Display$HdrCapabilities.HDR_TYPE_HLG,3,Android.Views.HdrType,Hlg,remove, | ||
| E,37,android/view/Display$HdrCapabilities.HDR_TYPE_HLG_PLUS,6,Android.Views.HdrType,HdrTypeHlgPlus,remove, | ||
| E,34,android/view/Display$HdrCapabilities.HDR_TYPE_INVALID,-1,Android.Views.HdrType,Invalid,remove, |
There was a problem hiding this comment.
HdrTypeHlgPlus includes the enum type name prefix, unlike existing Android.Views.HdrType members (Hdr10, Hdr10Plus, Hlg). Rename this member to match the established naming (e.g., HlgPlus).
There was a problem hiding this comment.
⚠️ Needs Changes
Reviewed the API-37 enumification changes across 4 files. Good cleanup of duplicate methodmap entries and useful additions to the documentation workflow.
Issues found
| Severity | Count | Summary |
|---|---|---|
| ❌ Error | 1 | Missing @api-since >= 37.0 scope on Tint set* parameter rule — would convert pre-37 methods (Drawable.setTint, Icon.setTint) to Color, causing an ABI break |
| 💡 Suggestion | 1 | Documentation of Color patterns is incomplete (doesn't mention Tint, fields, or set*-only restriction) |
Positive notes
- Good deduplication of ~20 redundant entries in
methodmap.csv - The three-tier scoping approach (package / type / member level) for Color rules is thorough
- The
string-length('Color')fix in XPath is clearer than the hardcoded5 - Useful new documentation for the namespace validation step and the
uniqworkflow
CI Status
CI builds are currently in progress. Cannot confirm pass/fail yet.
Generated by Android PR Reviewer for issue #11151 · ● 14.6M
| <attr api-since="37" path="/api/package | ||
| /*[(local-name()='class' or local-name()='interface')] | ||
| /method[starts-with(@name, 'set')] | ||
| /parameter[@type='int' and (starts-with(@name, 'tint') or substring(@name, string-length(@name) - string-length('Tint') + 1)='Tint')]" |
There was a problem hiding this comment.
🤖 ❌ Bugs & correctness — This Tint set* parameter rule is missing @api-since >= 37.0 on the method predicate, unlike every other rule in this third set (e.g., line 2191 for Color has /method[@api-since >= 37.0 and starts-with(@name, 'set')]).
Without the filter, this rule will match ALL set* methods across ALL API levels when building for API 37+. Specifically, Drawable.setTint(int tintColor) (API 21) and Icon.setTint(int tint) (API 23) would be incorrectly converted to Android.Graphics.Color, causing an ABI break for users upgrading to API 37.
Fix:
/method[`@api-since` >= 37.0 and starts-with(`@name`, 'set')]Rule: Consistent scoping of pattern-based metadata rules
| For API-37+, we're trying a "pattern-based" approach, wherein: | ||
|
|
||
| * method names which return `int` and end in `Color` | ||
| * method parameter names which start with `color` or end in `Color` | ||
|
|
||
| are automatically converted into `Android.Graphics.Color`. |
There was a problem hiding this comment.
🤖 💡 Documentation — This description doesn't fully match the metadata implementation:
- The metadata also converts fields (not just method returns/parameters)
- The metadata also adds Tint pattern matching (method returns ending in
Tint,set*parameters starting withtintor ending inTint) - Parameter conversion only applies to
set*methods, not all methods
Consider updating to reflect the full scope:
For API-37+, we're trying a "pattern-based" approach, wherein:
* fields named `color` or ending in `Color` (type `int`)
* method names which return `int` and end in `Color` or `Tint`
* `set*` method parameter names which start with `color`/`tint` or end in `Color`/`Tint`
are automatically converted into `Android.Graphics.Color`.Rule: Documentation accuracy
Context: http://github.com/jpobst/BindingStudio
Context: #10005
Use jpobst/BindingStudio to begin enumifying API-36.1.
Note: current dotnet/java-interop emits an "extra"
,onmap.csvoutput, which would make for a "noisy" diff (every line changed!). The diff size is reduced by removing trailing commas:This keeps the diff to a reasonable size.
TODO (lol): update
map.csvto current BindingStudio output after this is merged.WIP.
Current state: finished Creating enums.
Need to perform:
Commit Extract constants from API output.
Pull Request
title and
description
should follow the
commit-messages.mdworkflow documentation, and in particular should include: